Add CrosschainRemoteExecutor and GovernorCrosschain#6272
Add CrosschainRemoteExecutor and GovernorCrosschain#6272Amxx merged 19 commits intoOpenZeppelin:masterfrom
Conversation
🦋 Changeset detectedLatest commit: 1ed3b79 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request introduces two new abstract smart contracts for cross-chain communication: CrosschainRemoteController manages cross-chain linkage by storing gateway and counterpart addresses per chain and dispatching cross-chain messages through registered gateways, while CrosschainRemoteExecutor extends ERC7786Recipient to receive and process incoming cross-chain messages by decoding mode-encoded payloads and executing single calls, batch calls, or delegatecalls. A mock contract and comprehensive test suite are also added to support development and validation. Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @contracts/crosschain/CrosschainRemoteController.sol:
- Around line 26-33: Add an explicit validation in _crosschainExecute: call
getLink(chain) as before, then require that gateway != address(0) (and
optionally counterpart.length > 0) and revert with a descriptive error such as
LinkNotRegistered(chain) (from CrosschainLinked) before calling
IERC7786GatewaySource(gateway).sendMessage; this prevents calling the zero
address and surfaces a clear error when a chain link is unregistered.
🧹 Nitpick comments (1)
test/crosschain/CrosschainExecutor.test.js (1)
131-150: Consider specifying the expected revert reason for direct setter call.The crosschain call path correctly expects
FailedCall, but the direct call (line 133) uses a bare.to.be.revertedwithout specifying the expected error. This could mask unexpected revert reasons.♻️ Suggested improvement
- await expect(this.executor.$_setup(this.eoa.address, this.chain.toErc7930(this.newController))).to.be.reverted; + await expect(this.executor.$_setup(this.eoa.address, this.chain.toErc7930(this.newController))) + .to.be.revertedWithoutReason(); // EOA has no code, so supportsAttribute call failsOr if there's a specific custom error expected, use
revertedWithCustomError.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/crosschain/CrosschainRemoteController.solcontracts/crosschain/CrosschainRemoteExecutor.solcontracts/mocks/crosschain/CrosschainRemoteControllerMock.soltest/crosschain/CrosschainExecutor.test.js
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5914
File: contracts/crosschain/bridges/BridgeERC20.sol:57-58
Timestamp: 2025-10-03T13:14:57.679Z
Learning: In cross-chain bridge contracts like BridgeERC20, when processing incoming messages in _processMessage, avoid validation checks that revert on malformed addresses. Reverting would create cross-chain inconsistency where tokens are locked/burned on the source chain but never minted on the destination. Instead, use best-effort address extraction (e.g., address(bytes20(toBinary))) to maintain atomicity across chains. If tokens are minted to an incorrect address due to user error, recovery may be possible through admin controls rather than leaving funds permanently locked.
📚 Learning: 2025-10-03T13:14:57.679Z
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5914
File: contracts/crosschain/bridges/BridgeERC20.sol:57-58
Timestamp: 2025-10-03T13:14:57.679Z
Learning: In cross-chain bridge contracts like BridgeERC20, when processing incoming messages in _processMessage, avoid validation checks that revert on malformed addresses. Reverting would create cross-chain inconsistency where tokens are locked/burned on the source chain but never minted on the destination. Instead, use best-effort address extraction (e.g., address(bytes20(toBinary))) to maintain atomicity across chains. If tokens are minted to an incorrect address due to user error, recovery may be possible through admin controls rather than leaving funds permanently locked.
Applied to files:
contracts/crosschain/CrosschainRemoteController.solcontracts/crosschain/CrosschainRemoteExecutor.sol
📚 Learning: 2025-08-29T13:16:08.640Z
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5904
File: contracts/mocks/crosschain/ERC7786RecipientMock.sol:12-14
Timestamp: 2025-08-29T13:16:08.640Z
Learning: In OpenZeppelin contracts, mock contracts (like ERC7786RecipientMock) don't require input validation such as zero-address checks in constructors, as they are only used for testing purposes in controlled environments.
Applied to files:
contracts/mocks/crosschain/CrosschainRemoteControllerMock.solcontracts/crosschain/CrosschainRemoteExecutor.sol
📚 Learning: 2025-08-28T16:58:18.879Z
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5904
File: contracts/mocks/crosschain/ERC7786GatewayMock.sol:27-31
Timestamp: 2025-08-28T16:58:18.879Z
Learning: In the OpenZeppelin ERC7786 implementation, the `UnsupportedAttribute(bytes4 selector)` error is defined in the `IERC7786GatewaySource` interface, making it directly accessible to inheriting contracts without needing interface qualification.
Applied to files:
contracts/crosschain/CrosschainRemoteExecutor.sol
🧬 Code graph analysis (1)
test/crosschain/CrosschainExecutor.test.js (2)
test/crosschain/ERC7786Recipient.test.js (1)
getLocalChain(15-15)test/helpers/erc7579.js (6)
encodeMode(15-24)encodeSingle(26-27)CALL_TYPE_BATCH(12-12)encodeBatch(29-39)CALL_TYPE_DELEGATE(13-13)encodeDelegate(41-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Redirect rules - solidity-contracts
- GitHub Check: Header rules - solidity-contracts
- GitHub Check: Pages changed - solidity-contracts
- GitHub Check: tests
- GitHub Check: slither
- GitHub Check: tests-foundry
- GitHub Check: halmos
- GitHub Check: tests-upgradeable
- GitHub Check: coverage
🔇 Additional comments (13)
contracts/mocks/crosschain/CrosschainRemoteControllerMock.sol (1)
8-10: LGTM!Simple mock contract that correctly extends
CrosschainRemoteControllerand forwards the constructor parameters. Appropriate for testing purposes. Based on learnings, mock contracts don't require additional input validation.test/crosschain/CrosschainExecutor.test.js (4)
16-35: LGTM!The fixture correctly establishes the bidirectional link: the executor knows about the controller (via constructor), and the controller knows about the executor (via
$_setLink). Event verification during setup is good practice.
42-52: LGTM!Good verification of the bidirectional relationship between controller and executor. The use of
eventually.deep.equalfor the array comparison is appropriate.
54-95: LGTM!Comprehensive coverage of all three call types (single, batch, delegatecall) plus the invalid mode error path. The delegate mode test correctly expects
this.gatewayas the caller sincedelegatecallexecutes in the executor's context.
152-156: LGTM!Good verification that
reconfigureis restricted to self-calls only.contracts/crosschain/CrosschainRemoteExecutor.sol (5)
10-18: LGTM!State variables are appropriately private with public getters. The
CrosschainControllerSetevent captures both gateway and controller for proper observability.
20-22: LGTM!Constructor delegates to
_setup, ensuring consistent initialization and validation.
32-46: LGTM!The access control pattern (
msg.sender == address(this)) ensures reconfiguration can only happen through the executor itself, typically via a cross-chain call from the authorized controller. The gateway validation viasupportsAttributeis an elegant way to verify ERC-7786 compliance while also rejecting EOAs.
48-54: LGTM!Proper authorization check requiring both gateway instance match and controller sender match. The
equalcomparison fromByteslibrary handles the dynamic bytes comparison correctly.
57-74: Verify behavior with short payloads.If
payloadis shorter than 32 bytes, the slicepayload[0x00:0x20]will revert with an out-of-bounds error. This may be intentional (malformed messages should fail), but consider whether the revert message is clear enough for debugging cross-chain issues.contracts/crosschain/CrosschainRemoteController.sol (3)
14-18: LGTM!The mapping key is
bytesto support flexible chain identifiers. Theusingdeclarations for utility libraries are appropriate.
20-24: LGTM!Constructor correctly iterates through provided links with
allowOverride=false, ensuring no duplicate chain entries are silently overwritten during initialization.
51-64: LGTM!The
_setLinkimplementation correctly:
- Validates the gateway is ERC-7786 compliant
- Derives the chain identifier from the counterpart address
- Enforces single-registration semantics unless explicitly overriding
- Emits the appropriate event
The pattern of deriving the chain key from the counterpart simplifies the API and prevents mismatches.
There was a problem hiding this comment.
Questions:
- Do we want CrosschainRemoteExecutor to access calls from one vs multiple controllers ?
- Decision was made to leave like this.
- Is the CrosschainRemoteController really usefull, or should be rather have a Library with a function
- Would possibly result in loss of clarify/bad code separation
library CrosschainRemoteExecutorUtils {
function execute(
address gateway,
bytes memory executor,
Mode mode,
bytes memory executionCalldata
) internal returns (bytes32) {
return IERC7786GatewaySource(gateway).sendMessage(executor, abi.encodePacked(mode, executionCalldata), new bytes[](0));
}
}
| /// @dev Send crosschain instruction to an arbitrary remote executor via an arbitrary ERC-7786 gateway. | ||
| function relayCrosschain( | ||
| address gateway, | ||
| bytes memory executor, |
There was a problem hiding this comment.
What's your reasoning to allow specifying the gateway explicitly? It would be easy to batch together registerRemoteExecutor followed by relayCrosschain.
There was a problem hiding this comment.
I was thinking that in some cases you may want to use a custom target. Maybe there are two executor (a main one and a secondary) and you want to target one without reconfiguring. Adding/updating the default executor target is a storage operation that is expensive and that some people may want to skip.
Co-authored-by: Francisco Giordano <fg@frang.io>
frangio
changed the title
There was a problem hiding this comment.
As a side note, I think this remote executor could allow multichain accounts similar to Hyperlane's Interchain Accounts.
There was a problem hiding this comment.
Executor is indeed designed to be the "remote identity" of any "account". Can be used by a governor, but also by a timelock, a multisig, or any account
| if (callType == ERC7579Utils.CALLTYPE_SINGLE) { | ||
| executionCalldata.execSingle(execType); | ||
| } else if (callType == ERC7579Utils.CALLTYPE_BATCH) { | ||
| executionCalldata.execBatch(execType); | ||
| } else if (callType == ERC7579Utils.CALLTYPE_DELEGATECALL) { | ||
| executionCalldata.execDelegateCall(execType); | ||
| } else revert ERC7579Utils.ERC7579UnsupportedCallType(callType); |
There was a problem hiding this comment.
This pattern is shared with AccountERC7579. Maybe we should add a function to the ERC7579Utils:
function exec(bytes32 mode, bytes calldata executionCalldata) internal {
if (callType == CALLTYPE_SINGLE) {
executionCalldata.execSingle(execType);
} else if (callType == CALLTYPE_BATCH) {
executionCalldata.execBatch(execType);
} else if (callType == CALLTYPE_DELEGATECALL) {
executionCalldata.execDelegateCall(execType);
} else revert ERC7579UnsupportedCallType(callType);
}
There was a problem hiding this comment.
No strong opinion here. I'm not sure we really need to have this helper, but I also don't see any issues in adding it.
Co-authored-by: Francisco Giordano <fg@frang.io>
Co-authored-by: Ernesto García <ernestognw@gmail.com>
3 participants
Fixes #5903
PR Checklist
npx changeset add)